Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduces happy-path static test vectors #100

Merged
merged 16 commits into from
Oct 29, 2020

Conversation

nkohen
Copy link
Contributor

@nkohen nkohen commented Sep 29, 2020

Separated from their original PR #81 to narrow scope.

Continues work towards #47.

@nkohen nkohen added this to the v0.1 milestone Sep 29, 2020
@nkohen
Copy link
Contributor Author

nkohen commented Sep 29, 2020

Open comments from #81:

#81 (comment)
#81 (comment)
#81 (comment)

@benthecarman
Copy link
Contributor

ACK but I'm not writing the code using these :P

@nkohen
Copy link
Contributor Author

nkohen commented Sep 29, 2020

I think that the current test vector file is monolithic and might not be very usable for implementations that are just starting out in the future. I think I will add smaller tests (even though they will be redundant) for individual things in their own files like parsing LN messages, TLVs, Schnorr signning and sig-point computation, hash serialization, etc.

@nkohen
Copy link
Contributor Author

nkohen commented Sep 29, 2020

Oh also fee computation and tx construction

@nkohen
Copy link
Contributor Author

nkohen commented Sep 29, 2020

In short I'm a fan of the large monolithic test file because eventually it will be used for all sorts of edge cases (#47) but I also want small modular static tests for individual pieces that new clients may be implementing (e.g. you shouldn't need to have completed transaction construction and TLV parsing to test signature point computation)

@NicolasDorier
Copy link

NicolasDorier commented Oct 6, 2020

I am porting the tests:

  • dlc_schnorr_test.json
  • dlc_fee_test.json
  • dlc_hash_test.json
  • dlc_message_test.json SKIPPED: I think this decomposition is very useful. But not as a test, I would just add it as explanation in the .md.
  • dlc_test.json

@NicolasDorier
Copy link

RefundSig is serialized as DERSig + SigHash it should be compact signature of 64 bytes instead.

@NicolasDorier
Copy link

I think there is a problem in the serialization.
The third vector has Offer

a71a0006226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910ffda710c803f8cfbb27fc4201254f0068a232e600fbe3e81c7c637db107b07e6d62cc1ae900000000095ff28688d0a4aa9f2adf2df6fd5f8e56647ff3f1107dd22db62fc01515be5a129bfe89000000000bebc200d4f5207a6209f4e4e490d3c4fd279232c6b13511cacffb3e71f5d71d7f8ea69100000000000000000dfe1caa8af6fbba8b70e0df52c0499740a7cd995d4999d300b18495ac0cf7f300000000055c680f54d7d7537bdcc25d0fb4c4b273a151536afc25be44255791528c7114a529ff1b0000000006746250fda712403f201323e7bee139633fbd75c63ab1dede3fcab0b823dd5743e2b5f4dc81b1d6431fe7b5349cbc2eb36de636dc2e7254a363b852c45681e9573156fae96a63f00326027ae77c1b739b96cab8b80d0ee4e83266a3f7d4b7c5e757128efcdbb636b60016001477790e5bebfd206c6d3f98a7923e79271fb305a60000000005f5e1000001fda7146000520200000001b34e4345b6a07226f1aa91f2515096574fd6099eef1cd9885c8d069e5198cf760000000000000000000100c2eb0b00000000160014fa94fcdf2b201f6c31de85bf1e3922dc92e736050000000000000000ffffffff006b00000016001467e1a8c2f93d0c4cc08856cfc784b55c6433cbc0000000000000000500000064000000c8

The contractInfo should use the same order as the one in the vector.
But you can see that 0dfe1caa8af6fbba8b70e0df52c0499740a7cd995d4999d300b18495ac0cf7f3 is not after 03f8cfbb27fc4201254f0068a232e600fbe3e81c7c637db107b07e6d62cc1ae9.

@NicolasDorier
Copy link

I think there is also another problem in the way FundingSigs are serialized.

If the WitScript is [data_0][data_1], you are serializing [data_1][data_0]
You seem to inverse the order of the pushes.
For example, in FundingSigs of a P2WPKH, you serialize [pubkey] then [signature] instead of [signature] then [pubkey].

@nkohen nkohen merged commit 0b69c3e into discreetlogcontracts:master Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants